Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FastCDR version bump #141

Merged
merged 4 commits into from
Apr 5, 2024
Merged

Conversation

AlexDayCRL
Copy link
Contributor

the ros2 rolling repo recently bumped FastCDR from version 1.1.x to 2.2.x. This moves around some of the constants for FastBuffers. Specifically it renames getSerializedDataLength to get_serialized_data_length and moves where the DDS_CDR serialization flag lives.

References:
Rolling repos update: https://github.com/ros2/ros2/pull/1530/files
FastCDR Include Dirs @ 1.1.x: https://github.com/eProsima/Fast-CDR/tree/1.1.x/include/fastcdr
FastCDR Include Dirs @ 2.2.x: https://github.com/eProsima/Fast-CDR/tree/2.2.x/include/fastcdr

@AlexDayCRL
Copy link
Contributor Author

this is also stacked on top of yadu/noble and rebased to rolling.

@Yadunund
Copy link
Member

Yadunund commented Mar 27, 2024

Thanks for spear heading this initiative. One thing to keep in mind is that we need the code to compile for Iron on Jammy with FastCDR 1.x.x and Rolling on Noble with FastCDR 2.x.x. We can achieve this in two ways

  1. Branch off rolling to create an iron branch for rmw_zenoh. Then merge this into rolling. All future PRs merged into rolling will need to be carefully backported to iron.
  2. We implement this change by
  • Checking version of FastCDR found a find_package() call within rmw_zenoh_cpp/CMakeLists.txt and then injecting a compile definition into the codebase, say -DFAST_CDR_2 if 2.x.x is found.
  • Updating rmw_zenoh.cpp such that it relies on #ifdef FAST_CDR_2 blocks to invoke the 2.x.x API or else the 1.x.x ones.

Personally I'm in favor on 2 since rmw_zenoh is still under heavy development and I rely on Iron binaries for Nav2, moveit, open-rmf, etc to test functionality. @clalancette what do you think?

AlexDayCRL and others added 3 commits April 4, 2024 10:57
@clalancette clalancette force-pushed the fast-cdr-version-bump branch from 7a59970 to 8dee384 Compare April 4, 2024 16:01
@clalancette
Copy link
Collaborator

So I rebased this onto the latest, and I also went with strategy 2 as laid out by @Yadunund . For now, we can keep all of our development on the rolling branch, though eventually we are going to want to branch off for iron (and soon jazzy). In any case, what I did in 8dee384 should paper over those differences, and in my testing, it makes this build for both Fast-CDR v1 and v2.

Please take a look.

@clalancette clalancette self-assigned this Apr 4, 2024
Copy link
Member

@Yadunund Yadunund left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Tested locally with rolling from source and Fast-CDR 2.x.
I'm surprised we didn't need to include a add_compile_definitions(-DFASTCDR_VERSION_MAJOR ${FASTCDR_MAJOR_VERSION}) in the CMakeLists.txt file.

@Yadunund Yadunund merged commit 58404f7 into ros2:rolling Apr 5, 2024
5 of 6 checks passed
@clalancette
Copy link
Collaborator

I'm surprised we didn't need to include a add_compile_definitions(-DFASTCDR_VERSION_MAJOR ${FASTCDR_MAJOR_VERSION}) in the CMakeLists.txt file.

Fast-CDR already generates this during its own installation phase and puts it in config.h file, which we can then reuse. So no need for our own definition.

@Yadunund
Copy link
Member

Yadunund commented Apr 5, 2024

I'm surprised we didn't need to include a add_compile_definitions(-DFASTCDR_VERSION_MAJOR ${FASTCDR_MAJOR_VERSION}) in the CMakeLists.txt file.

Fast-CDR already generates this during its own installation phase and puts it in config.h file, which we can then reuse. So no need for our own definition.

Ah that explains it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants